-
Notifications
You must be signed in to change notification settings - Fork 1
fix: Polling after initial cached config #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
throwOnFailedInitialization = false, | ||
skipInitialPoll = false, | ||
} = this.configurationRequestParameters; | ||
if (pollingIntervalMs <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a reasonable guard/error-action to take here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can still log the error, and then perhaps set it to the default? Alternatively, we could use an invalid polling interval to mean don't poll, which would be consistent with other SDKs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think setting to default is the way to go. Since there are pollAfterSuccessfulInitialization
and pollAfterFailedInitialization
, it doesn't make sense for an invalid argument to implicitly override the variables explicitly controlling this behaviour.
}, | ||
); | ||
const pollingCallback = async () => { | ||
if (await this.flagConfigurationStore.isExpired()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Polling = frequent checking of the cache and, if necessary, a fetch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for jumping on this!
expect(variation).toBe(pi); | ||
}); | ||
|
||
describe('Poll after successful start', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
throwOnFailedInitialization = false, | ||
skipInitialPoll = false, | ||
} = this.configurationRequestParameters; | ||
if (pollingIntervalMs <= 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can still log the error, and then perhaps set it to the default? Alternatively, we could use an invalid polling interval to mean don't poll, which would be consistent with other SDKs
}, | ||
); | ||
const pollingCallback = async () => { | ||
if (await this.flagConfigurationStore.isExpired()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
fixes FF-3485
labels: mergeable
Fixes: #FF-3485
Motivation and Context
The initial fetch logic of the client ignores polling configuration when the cache is unexpired.
Description
How has this been tested?
Unit tests